Add sanitizer to disable auto lightbox #6936
Conversation
|
Plugin builds for 7a3d259 are ready 🛎️!
|
includes/amp-helper-functions.php
Outdated
| @@ -1608,6 +1608,12 @@ function amp_get_content_sanitizers( $post = null ) { | |||
| ); | |||
| } | |||
|
|
|||
| $is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', true ); | |||
|
|
|||
| if ( ! $is_auto_lightbox_disabled ) { | |||
There was a problem hiding this comment.
This is an opposite of what should be. If $is_auto_lightbox_disabled is true, we should use AMP_Auto_Lightbox_Disable_Sanitizer.
There was a problem hiding this comment.
Yes, I think it should be vice versa.
However current implementation is based on the Implementation brief of the issue.
There was a problem hiding this comment.
What I meant here is that the variable name says is_auto_lightbox_disabled and when it was true (i.e. disabled) we actually enabled the auto lightbox feature - so the opposite.
There was a problem hiding this comment.
What I meant here is that the variable name says is_auto_lightbox_disabled and when it was true (i.e. disabled) we actually enabled the auto lightbox feature - so the opposite.
includes/amp-helper-functions.php
Outdated
| @@ -1608,6 +1608,12 @@ function amp_get_content_sanitizers( $post = null ) { | |||
| ); | |||
| } | |||
|
|
|||
| $is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', false ); | |||
There was a problem hiding this comment.
Shouldn't it be true by default? The acceptance criteria in #5122 says:
By default the
data-amp-auto-lightbox-disableattribute should be added to thebody.
The code above doesn't add the data-amp-auto-lightbox-disable attribute by default.
There was a problem hiding this comment.
I'm late to respond to #5122 (comment).
After thinking about it some more, I think it's probably good to indeed disable by default. Otherwise, users who see the Lightbox toggle will be confused what purpose it serves if the images are lightboxed anyway.
| $is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', false ); | |
| $is_auto_lightbox_disabled = apply_filters( 'amp_auto_lightbox_disabled', true ); |
| $this->assertContains( | ||
| AMP_Auto_Lightbox_Disable_Sanitizer::class, | ||
| array_keys( amp_get_content_sanitizers() ) | ||
| ); | ||
|
|
||
| remove_all_filters( 'amp_auto_lightbox_disabled' ); | ||
|
|
||
| $this->assertNotContains( | ||
| AMP_Auto_Lightbox_Disable_Sanitizer::class, | ||
| array_keys( amp_get_content_sanitizers() ) | ||
| ); |
There was a problem hiding this comment.
| $this->assertContains( | |
| AMP_Auto_Lightbox_Disable_Sanitizer::class, | |
| array_keys( amp_get_content_sanitizers() ) | |
| ); | |
| remove_all_filters( 'amp_auto_lightbox_disabled' ); | |
| $this->assertNotContains( | |
| AMP_Auto_Lightbox_Disable_Sanitizer::class, | |
| array_keys( amp_get_content_sanitizers() ) | |
| ); | |
| $this->assertArrayHasKey( | |
| AMP_Auto_Lightbox_Disable_Sanitizer::class, | |
| amp_get_content_sanitizers() | |
| ); | |
| remove_all_filters( 'amp_auto_lightbox_disabled' ); | |
| $this->assertArrayNotHasKey( | |
| AMP_Auto_Lightbox_Disable_Sanitizer::class, | |
| amp_get_content_sanitizers() | |
| ); |
| * @return void | ||
| */ | ||
| public function sanitize() { | ||
| $this->dom->body->setAttributeNode( $this->dom->createAttribute( 'data-amp-auto-lightbox-disable' ) ); |
There was a problem hiding this comment.
Very good to use createAttribute as then no attribute value will be shown. 👍
…nt/5122-disabled-auto-lightbox * 'develop' of github.com:ampproject/amp-wp: (181 commits) Copy referrerpolicy attribute from img to amp-pixel Improve nav menu E2E tests by creating test menu before each test suite Bail out if there is no menu location or it is already selected Add E2E tests for Twenty Twenty-Two header nav menu block Test Twenty Twenty search modal on mobile breakpoint Remove redundant space Use `Extension` class constant for getting `amp-pixel` tag name Add missing replacement string to `str_replace` and reformat Use `::class` constant instead of hardcoded class name Examine parsed URL instead of using regex Remove `amp-pixel` from selector conversion mapping Bump dependabot/fetch-metadata from 1.1.1 to 1.3.0 Improve strings to account for one or more issues Fix type check by passing explicit string as href prop Include SCSS files in the lint-staged config for stylelint Fix stylelint issue Replace `img` with `amp-pixel` when dealing with Facebook tracking pixel Rename 'allowed' to 'associated' Use named functions Add label for why the validation data is being shown ...
westonruter
changed the title
Co-authored-by: Dhaval Parekh <[email protected]>
westonruter
added
the
Changelogged
Summary
Fixes #5122
Add sanitiser to disable auto lightbox by adding
data-amp-auto-lightbox-disableattribute to body element.Checklist